chore(lint): add a lint to generate a json file with all enum, struct…#3360
chore(lint): add a lint to generate a json file with all enum, struct…#3360SouchonTheo wants to merge 1 commit intomainfrom
Conversation
93a718c to
d6739cd
Compare
|
ℹ️ Backward-compat snapshot base files were updated Only new types/variants/upgrades were added. This is expected when introducing new versioned types. ➕ Additions
|
575e26d to
26d13e3
Compare
nsarlin-zama
left a comment
There was a problem hiding this comment.
some suggestions, but this is a really good job !
@nsarlin-zama partially reviewed 12 files and all commit messages, and made 17 comments.
Reviewable status: 12 of 29 files reviewed, 16 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).
.linelint.yml line 13 at r1 (raw file):
- coverage - utils/tfhe-lints/**/main.stderr - /**/*.json
maybe should be scoped to the path where the files are generated?
.github/workflows/main_snapshot_change_check.yml line 50 at r1 (raw file):
run: | mismatch=0 for head_file in utils/tfhe-lints/snapshots/head/*_head.json; do
maybe this could be a script in the ci/ folder, would make it easier to maintain on the long term
utils/tfhe-backward-compat-checker/src/main.rs line 31 at r1 (raw file):
} type Registry = BTreeMap<String, EnumSnapshot>;
nitpick, this could be a newtype: struct Registry(BTreeMap<String, EnumSnapshot>). That way you could define true methods on it for load, get_additions, get_modifications,...
utils/tfhe-backward-compat-checker/src/main.rs line 151 at r1 (raw file):
} fn get_additions(old: &Registry, new: &Registry) -> Vec<String> {
I don't know if based on the number of enum we have perf are important here (I guess not), but it could be made faster by doing only one traversal of both registries for additions, modifications and removal.
Since you use a btree map which is ordered (or even you could instead use a sorted vec) you can iterate on both registries at the same time (manually using .next()), and then if old > new, there is a removal, if new > old there is an addition, if new == old you check for modifs.
This is just an idea if perf matters, no need to change anything if it is already fast enough.
utils/tfhe-lints/Makefile.compat line 9 at r1 (raw file):
lint-tfhe = $(call DYLINT_CMD,$(1),$(2)) -p tfhe -- --features=boolean,shortint,integer,strings,zk-pok lint-tfhe-zk-pok = $(call DYLINT_CMD,$(1),$(2)) -p tfhe-zk-pok -- --features=experimental
did you look at other crates that might use this ? Maybe tfhe-csprng ?
utils/tfhe-lints/Makefile.compat line 32 at r1 (raw file):
lint-generate-report:
I think it would be clearer if this target and lint check below were named something like "backward-snapshot-generate", and "backward-snapshot-check"? "link-check" makes it sound like it checks the lints. This is implemented as a lint but it is an implementation detail.
utils/tfhe-lints/README.md line 130 at r1 (raw file):
make -f utils/tfhe-lints/Makefile.compat lint-generate SUFFIX=my_branch
is this lint-generate-report ? Or I'm missing something in the Makefile?
utils/tfhe-lints/lints/Cargo.toml line 5 at r1 (raw file):
version = "0.1.0" description = "Project specific lints for TFHE-rs" edition = "2021"
could be updated to 2024 I guess ?
utils/tfhe-lints/snapshot/Cargo.toml line 5 at r1 (raw file):
version = "0.1.0" description = "Backward-compatibility snapshot lint for TFHE-rs (VersionsDispatch metadata collection)" edition = "2021"
2024
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 18 at r1 (raw file):
#[derive(Serialize, Clone)] pub struct LintVariantMeta {
Why is this prefixed by "Lint" ?
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 92 at r1 (raw file):
/// /// ### Why is this needed? /// Proc macros only see token-level type names (e.g. `ServerKey`) without full paths, causing
I think the first sentence about proc macro is a bit confusing (someone reading this in the future might not know that you first implemented it with a proc macro). I think the next sentence alone is enough
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 109 at r1 (raw file):
/// For structs: hashes `"field_name:field_type;"` for each field. /// For enums: hashes `"variant_name:field_types;"` for each variant. fn compute_struct_hash<'tcx>(
I'd call this compute_type_hash since it handles enum and structs
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 116 at r1 (raw file):
let mut hasher = Sha256::new(); if adt_def.is_enum() {
Some corner cases to check:
- in rust, types can be struct, enum or unions: https://doc.rust-lang.org/reference/items/unions.html. Do you think they are handled correctly? Since you use "all_fields", it might also cover union
- struct can have "unnamed" fields, (this is usually called a tuple). Will those appear in all_fields ? Maybe try defining a tuple, change the type and check that the hash is modified
- similarly, enum variants can also be named or unnamed (basically, enum variants are structs), so it could make sense to handle them like structs.
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 146 at r1 (raw file):
target_str: &str, ) -> String { let ItemKind::Impl(impl_block) = &impl_item.kind else {
This function could directly take the impl_block as parameter to have static guarantees that this can never fail
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 156 at r1 (raw file):
let impl_item = cx.tcx.hir_impl_item(impl_item_id); if impl_item.ident.as_str() != "upgrade" {
could be a const
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 316 at r1 (raw file):
fn handle_impl<'tcx>(&self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { // Check if this impl is for the Upgrade trait let ItemKind::Impl(impl_block) = &item.kind else {
here also you could avoid this match by having the impl_block as parameter
IceTDrinker
left a comment
There was a problem hiding this comment.
Very quick read for now on my end (day has been long), I'll take more time tomorrow to dig in the details
so as I often do lately : current comments are for stuff around the code and don't address the code itself
still as said elsewhere : looking like a very good basis :)
@IceTDrinker partially reviewed 11 files and all commit messages, and made 6 comments.
Reviewable status: 22 of 29 files reviewed, 19 unresolved discussions (waiting on nsarlin-zama, soonum, and SouchonTheo).
utils/tfhe-lints/Makefile.compat line 9 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
did you look at other crates that might use this ? Maybe tfhe-csprng ?
agreed, the list of crates could be found programmatically by scanning the Cargo.toml files and seeing who has tfhe-versionable as a dependency
utils/tfhe-lints/lints/Cargo.toml line 5 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
could be updated to 2024 I guess ?
if it compiles right away : yes, otherwise don't bother in this PR, some patterns don't work properly in 2024 (due to some changes to temporary lifetimes)
can be done in a follow up if it causes compilation issues
Cargo.toml line 25 at r1 (raw file):
"utils/wasm-par-mq", "utils/wasm-par-mq/examples/msm", "utils/wasm-par-mq/web_tests",
what happened here ?
in general I prefer to avoid "unnecessary" updates to some files in a given PR, because it can:
- hide issues
- make the history contain "unrelated" commits
utils/tfhe-lints/Makefile.compat line 11 at r1 (raw file):
lint-tfhe-zk-pok = $(call DYLINT_CMD,$(1),$(2)) -p tfhe-zk-pok -- --features=experimental .PHONY: lint-base lint-head lint-check
I would keep the .PHONY near their targets
makes it easier to not forget
also we use it for help (try: make help), but not sure it would work if this Makefile is included in another one
utils/tfhe-lints/Makefile.compat line 34 at r1 (raw file):
lint-generate-report: cargo run -p tfhe-backward-compat-checker -- diff-report \ --old-dir $(BASE_DIR) --old-suffix $(BASE_FILE) \
nit: looks like a mix of tab/spaces
26d13e3 to
4099e11
Compare
SouchonTheo
left a comment
There was a problem hiding this comment.
Ready to be review again
@SouchonTheo made 14 comments and resolved 6 discussions.
Reviewable status: 22 of 29 files reviewed, 13 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and soonum).
.linelint.yml line 13 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
maybe should be scoped to the path where the files are generated?
making sense
Cargo.toml line 25 at r1 (raw file):
Previously, IceTDrinker wrote…
what happened here ?
in general I prefer to avoid "unnecessary" updates to some files in a given PR, because it can:
- hide issues
- make the history contain "unrelated" commits
Each time I rebase I have issue
so I did a sort on all entries
it avoid rebase issue but not neccessary
.github/workflows/main_snapshot_change_check.yml line 50 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
maybe this could be a script in the ci/ folder, would make it easier to maintain on the long term
you are absolutely right
utils/tfhe-backward-compat-checker/src/main.rs line 31 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
nitpick, this could be a newtype:
struct Registry(BTreeMap<String, EnumSnapshot>). That way you could define true methods on it for load, get_additions, get_modifications,...
you are right !
utils/tfhe-backward-compat-checker/src/main.rs line 151 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
I don't know if based on the number of enum we have perf are important here (I guess not), but it could be made faster by doing only one traversal of both registries for additions, modifications and removal.
Since you use a btree map which is ordered (or even you could instead use a sorted vec) you can iterate on both registries at the same time (manually using.next()), and then if old > new, there is a removal, if new > old there is an addition, if new == old you check for modifs.This is just an idea if perf matters, no need to change anything if it is already fast enough.
this is already super fast (fast enough for generating a file lol)
but you are right if we can do better I will try to do it !
utils/tfhe-lints/Makefile.compat line 9 at r1 (raw file):
Previously, IceTDrinker wrote…
agreed, the list of crates could be found programmatically by scanning the Cargo.toml files and seeing who has tfhe-versionable as a dependency
good catch will take a look
utils/tfhe-lints/Makefile.compat line 11 at r1 (raw file):
Previously, IceTDrinker wrote…
I would keep the .PHONY near their targets
makes it easier to not forget
also we use it for help (try: make help), but not sure it would work if this Makefile is included in another one
I made an update on the make help at the root
I also made the change requested
utils/tfhe-lints/Makefile.compat line 32 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
I think it would be clearer if this target and lint check below were named something like "backward-snapshot-generate", and "backward-snapshot-check"? "link-check" makes it sound like it checks the lints. This is implemented as a lint but it is an implementation detail.
you are right, I will do the change
utils/tfhe-lints/Makefile.compat line 34 at r1 (raw file):
Previously, IceTDrinker wrote…
nit: looks like a mix of tab/spaces
good catch
utils/tfhe-lints/README.md line 130 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
is this lint-generate-report ? Or I'm missing something in the Makefile?
you are right the md is super outdated
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 116 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
Some corner cases to check:
- in rust, types can be struct, enum or unions: https://doc.rust-lang.org/reference/items/unions.html. Do you think they are handled correctly? Since you use "all_fields", it might also cover union
- struct can have "unnamed" fields, (this is usually called a tuple). Will those appear in all_fields ? Maybe try defining a tuple, change the type and check that the hash is modified
- similarly, enum variants can also be named or unnamed (basically, enum variants are structs), so it could make sense to handle them like structs.
you are right the corner case was the field name that I ignore
I made a fix for it
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 156 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
could be a const
Done.
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 316 at r1 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
here also you could avoid this match by having the impl_block as parameter
I did it but I kept item for the owner id
|
Previously, SouchonTheo (Theo Souchon) wrote…
maybe cargo metadata can be waht you are looking for |
4099e11 to
916528b
Compare
nsarlin-zama
left a comment
There was a problem hiding this comment.
@nsarlin-zama partially reviewed 20 files and all commit messages, made 3 comments, and resolved 7 discussions.
Reviewable status: 30 of 31 files reviewed, 7 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).
utils/tfhe-backward-compat-checker/src/main.rs line 151 at r1 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
this is already super fast (fast enough for generating a file lol)
but you are right if we can do better I will try to do it !
Nice!
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 116 at r1 (raw file):
Nice! Looks like it handles all cases, from the rustc doc:
structs, tuples, and unionss are considered to have a single variant with variant index zero, aka FIRST_VARIANT.`
Maybe you can add a comment about that to explain that it works on struct too.
Did you confirm that you go through all the fields for tuples also? Will field_name be empty in that case?
utils/tfhe-backward-compat-checker/src/main.rs line 104 at r2 (raw file):
eprintln!("Cannot read directory {}: {}", dir.display(), err); eprintln!("Return empty registry"); return Registry(snapshots);
Is it ok to return an empty registry if read_dir fails? Maybe we should propagate the error instead?
916528b to
5c48b21
Compare
a3554c8 to
a9c1977
Compare
nsarlin-zama
left a comment
There was a problem hiding this comment.
@nsarlin-zama partially reviewed 20 files and all commit messages, made 2 comments, and resolved 2 discussions.
Reviewable status: 29 of 34 files reviewed, 22 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).
.github/workflows/pr_snapshot_change_warning.yml line 45 at r4 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
I did it in a way to use less possible variable in the makefile but it's so bad at the end :/
Maybe the best way is to use variable dedicated for each rule and that's it we remove all argument and we update the ci
wdyt ?
What looks interesting to see here imo is the BASE_SNAPSHOT_DIR, not HEAD_SNAPSHOT_DIR. I think it makes sens to set this one here because BASE_SNAPSHOT_DIR is not inside the head folder, so if we change where it's cloned in this workflow it will not work anymore.
So I think it should be set as a parameter from this workflow.
Since we run from the head folder, everything related to head is "self contained" so it make sense to have it in the makefile, but everything related to base is not so I think it should be defined at the workflow level.
utils/tfhe-backward-compat-checker/src/diff.rs line 148 at r8 (raw file):
} /// Single source of truth for severity rules.
nice!
nsarlin-zama
left a comment
There was a problem hiding this comment.
@nsarlin-zama made 1 comment.
Reviewable status: 29 of 34 files reviewed, 22 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 184 at r6 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
This happen if there is no enum under version dispatch trait I guess
we could say that's an error but this is also valid if we run it on a crate without tfhe-versionize
wdyt ?
yes I think it's better if the lint works even on crates without versioning enums
IceTDrinker
left a comment
There was a problem hiding this comment.
Looking much better, still some things to work out in the CI workflows
I think there is still some confusion in at least one workflow on what base is
So naming:
base_branch/merge_targer : where we are merging to nearly 100% of the time == main
base : "the committed snapshots in a PR" == this PR for example
head : "snapshots that were just generated to check if stuff changed" == in CI to verify the snapshots did not change
@IceTDrinker partially reviewed 16 files and all commit messages, made 41 comments, and resolved 17 discussions.
Reviewable status: all files reviewed, 40 unresolved discussions (waiting on nsarlin-zama, soonum, and SouchonTheo).
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 184 at r6 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
This happen if there is no enum under version dispatch trait I guess
we could say that's an error but this is also valid if we run it on a crate without tfhe-versionize
wdyt ?
maybe could emit a warning, since we ask stuff about versioning, if we don't find anything about versioning it's a bit suspicious
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 201 at r6 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
in theory no
I did both but only source for example is enough
we are just matching the upgrade with the corresponding full enum_name
do you think it's necessary to comment it ?
I think it would be good to error out if we have one and not the other
has_source != has_target
would be the corresponding weid case
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 325 at r6 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
it's not really different
in fine if we are something else than the first one we don't really care either :/
(same logic primitive type)
mb I will put a return instead
I see another format below, it's just that I'm not clear as to what the non Adt case may be, and why you had a fallback here and a fallback below that is still present
utils/tfhe-lints/Makefile.compat line 9 at r1 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
you are right it could be a good reminder for future project let keep that in mind
to not forget : a second small PR to check we run the correct checks for the correct crates
utils/tfhe-backward-compat-checker/src/diff.rs line 20 at r8 (raw file):
#[derive(Debug, Clone)] pub enum DiffEntry { EnumAdded {
Enum : it's a VersionsDispatch enum ?
add a doc comment on each to explain what it represents
utils/tfhe-backward-compat-checker/src/diff.rs line 26 at r8 (raw file):
EnumRemoved { name: String, },
no EnumHashChanged ?
no VariantHashChanged ? (no idea if it's relevant)
could be good having comments explaining why we don't have some potentially
utils/tfhe-backward-compat-checker/src/diff.rs line 32 at r8 (raw file):
type_path: String, }, VariantRemoved {
variant : a variant from a VersionsDispatch enum ?
utils/tfhe-backward-compat-checker/src/diff.rs line 37 at r8 (raw file):
type_path: String, }, StructHashChanged {
struct that is versioned ?
utils/tfhe-backward-compat-checker/src/diff.rs line 44 at r8 (raw file):
new_hash: String, }, UpgradeAdded {
Upgrade code between two versions of a type ?
utils/tfhe-backward-compat-checker/src/diff.rs line 141 at r8 (raw file):
.filter(|e| e.severity() == Severity::Warning) .collect(); let additions = entries
neutral
maybe ?
is it always an addition if it's Ok ?
utils/tfhe-backward-compat-checker/src/diff.rs line 150 at r8 (raw file):
/// Single source of truth for severity rules. pub fn severity(&self) -> Severity { match self {
a few comments to give an example of why it might be a warning and not a hard error, etc. could be nice I think
since this is the part that defines the rules on what is ok/warn/err
utils/tfhe-backward-compat-checker/src/diff.rs line 265 at r8 (raw file):
}; if !file_name.starts_with("lint_enum_snapshots_") || !file_name.ends_with(".json") {
I don't think I asked to error on "unpredicted file", I think it's fine to ignore like you did before
utils/tfhe-backward-compat-checker/src/diff.rs line 306 at r8 (raw file):
loop { match (old_iter.peek(), new_iter.peek()) {
thanks a lot for the comments, it makes things much more readable 🙏
utils/tfhe-backward-compat-checker/src/diff.rs line 331 at r8 (raw file):
// for modifications // We move both iterators forward and compare the enums, but we don't // record anything
we do record a diff I think ?
since we seem to pass &mut entries to diff_enum
utils/tfhe-backward-compat-checker/src/report.rs line 10 at r8 (raw file):
} let (errors, warnings, additions) = DiffEntry::split_by_severity(entries);
errors, warnings, neutral
maybe ? not sure how it's called generally
utils/tfhe-backward-compat-checker/src/report.rs line 17 at r8 (raw file):
lines.push(":x: **Backward-compat snapshot: breaking changes detected**\n".to_string()); lines.push( "Variant removals were detected. This **will break deserialization** \
you don't check here if there were variants removals, today errors are only variants removal but I don't think we should specialize the logs if we don't do an analysis here
it's perfectly fine to say "errors detected, see: ... blablabla" to have the detail
utils/tfhe-backward-compat-checker/src/report.rs line 26 at r8 (raw file):
); lines.push( "Upgrades or struct definitions were modified or removed. \
same
utils/tfhe-backward-compat-checker/src/report.rs line 35 at r8 (raw file):
); lines.push( "Only new types/variants/upgrades were added. \
same "neutral changes detected, see full log:"
utils/tfhe-backward-compat-checker/src/report.rs line 76 at r8 (raw file):
lines.push("| Change | Severity |".to_string()); lines.push("|--------|----------|".to_string()); lines.push("| New enum/variant/upgrade | :heavy_check_mark: OK |".to_string());
prefer using your mapping in code that exists for the various variants, here you end up with a different source of truth which will get out of sync
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 119 at r8 (raw file):
for variant in adt_def.variants() { let name = tcx.def_path_str(variant.def_id);
why this change ?
probably not important, just curious, I don't know the internal rust compiler APIs
.github/workflows/backward_compat_main_snapshot_consistency.yml line 30 at r8 (raw file):
jobs: snapshot-change-check: name: backward_compat_main_snapshot_consistency/snapshot-consistency
careful, the name does not match the job
normally they should match IIRC, check with @soonum there may be a notion page about how we name things org wide
.github/workflows/backward_compat_main_snapshot_consistency.yml line 45 at r8 (raw file):
uses: dtolnay/rust-toolchain@e97e2d8cc328f1b50210efc529dca0028893a2d9 # zizmor: ignore[stale-action-refs] with: toolchain: nightly-2026-01-22
is it the toolchain we need ?
there could be a make target that prints it for the first rust/rustup install, this way it is always in sync
.github/workflows/backward_compat_main_snapshot_consistency.yml line 54 at r8 (raw file):
- name: Check snapshot consistency run: | ./scripts/check_snapshot_consistency.sh
wondering if this script should have inputs
it should at least verify that the dirs exist
.github/workflows/backward_compat_pr_change_report.yml line 20 at r8 (raw file):
jobs: snapshot-change-warning: name: backward_compat_pr_change_report/change-report
naming convention to check with @soonum
.github/workflows/backward_compat_pr_change_report.yml line 36 at r8 (raw file):
persist-credentials: 'false' ref: ${{ github.event.pull_request.base.sha }} path: base
I think base is never used, so there is a problem in the logic of the workflow
I think the CI is doing the following :
do a report in the head dir between a head snapshot and the PR branch
while you might want (?)
do a report somewhere between the base branch snapshot and the PR branch snapshot
.github/workflows/backward_compat_pr_change_report.yml line 45 at r8 (raw file):
- name: Generate diff report id: report working-directory: head
maybe prefer cd in the script, I was lost
.github/workflows/backward_compat_pr_snapshot_consistency.yml line 33 at r8 (raw file):
jobs: backward-compat-schema: name: backward_compat_pr_snapshot_consistency/snapshot-consistency (bpr)
naming convention
.github/workflows/backward_compat_pr_snapshot_consistency.yml line 34 at r8 (raw file):
backward-compat-schema: name: backward_compat_pr_snapshot_consistency/snapshot-consistency (bpr) if: (github.event_name == 'push' && github.repository == 'zama-ai/tfhe-rs') ||
this if should be removed
.github/workflows/backward_compat_pr_snapshot_consistency.yml line 36 at r8 (raw file):
if: (github.event_name == 'push' && github.repository == 'zama-ai/tfhe-rs') || github.event_name != 'push' runs-on: "runs-on=${{ github.run_id }}/runner=cpu-small"
why runs-on ? is it something that is heavy ?
.github/workflows/backward_compat_pr_snapshot_consistency.yml line 38 at r8 (raw file):
runs-on: "runs-on=${{ github.run_id }}/runner=cpu-small" concurrency: group: ${{ github.workflow_ref }}${{ github.ref == 'refs/heads/main' && github.sha || '' }}
concurrency is made for a push to main workflow, to be removed/reworked
.github/workflows/backward_compat_pr_snapshot_consistency.yml line 39 at r8 (raw file):
concurrency: group: ${{ github.workflow_ref }}${{ github.ref == 'refs/heads/main' && github.sha || '' }} cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}
true for PR trigger, to check with @soonum
.github/workflows/backward_compat_pr_snapshot_consistency.yml line 45 at r8 (raw file):
with: persist-credentials: 'false' token: ${{ env.CHECKOUT_TOKEN }}
why do we need a checkout token here but not in the other workflows ?
perhaps to be reworked
.github/workflows/backward_compat_pr_snapshot_consistency.yml line 50 at r8 (raw file):
uses: dtolnay/rust-toolchain@e97e2d8cc328f1b50210efc529dca0028893a2d9 # zizmor: ignore[stale-action-refs] this action doesn't create releases with: toolchain: nightly-2026-01-22
use a make command for this
.github/workflows/backward_compat_pr_snapshot_consistency.yml line 59 at r8 (raw file):
- name: Check snapshot consistency run: | ./scripts/check_snapshot_consistency.sh
I think this workflow could be shared with the first one ?
since they seem to be doing similar stuff, but once on PR, once on merge to main
utils/tfhe-lints/Makefile line 7 at r8 (raw file):
cargo dylint --lib tfhe_lints_snapshot --no-deps lint-tfhe = $(call DYLINT_CMD,$(1)) -p tfhe -- --features=boolean,shortint,integer,strings,zk-pok
even though it calls dyling, this target is made to snapshot the crates, so maybe call the three targets
snapshot-$crate
utils/tfhe-lints/Makefile line 12 at r8 (raw file):
# Run all snapshot lints for a given directory define run-all-snapshot-lints
the fact lints are used to generate snapshots is an implementation details, so I would call this run-all-snapshots
the comment can have information about the fact it uses dylint to run the snapshot
utils/tfhe-lints/Makefile line 47 at r8 (raw file):
$(run-all-lints) .PHONY: backward-snapshot-base # Generate the base snapshot. Should be called when you modified enums, structs or upgrade related to Versioned traits
Versioned types rather ?
utils/tfhe-lints/Makefile line 55 at r8 (raw file):
backward-snapshot-head: install_cargo_dylint @echo "Generating head snapshot" @rm -rf $(SNAPSHOT_DIR)/head
is that HEAD_SNAPSHOT_DIR ?
same below
utils/tfhe-lints/Makefile line 61 at r8 (raw file):
# This variable is set for the CI # You might need to change it for a local usage BASE_SNAPSHOT_DIR ?= ../base/$(SNAPSHOT_DIR)
this path looks to be incorrect ?
I don't see a base dir in your commit I think ?
utils/tfhe-lints/snapshots/lint_enum_snapshots_tfhe_csprng_base.json line 9 at r2 (raw file):
"inner_type_def_path": "generators::aes_ctr::index::AesIndex", "inner_type_display": "generators::aes_ctr::index::AesIndex", "struct_hash": "923080edce7f6ce44eaf6688f154ac9c387ae9cfd0ae5a4175054c0dbb7853bb"
why did the hash change ? I don't recall if you updated the way structs are hashed
nsarlin-zama
left a comment
There was a problem hiding this comment.
@nsarlin-zama made 2 comments.
Reviewable status: all files reviewed, 40 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).
utils/tfhe-backward-compat-checker/src/diff.rs line 20 at r8 (raw file):
Previously, IceTDrinker wrote…
Enum : it's a VersionsDispatch enum ?
add a doc comment on each to explain what it represents
yes maybe being explicit and calling this VersionsDispatchEnumChanged would be clearer
utils/tfhe-backward-compat-checker/src/diff.rs line 37 at r8 (raw file):
Previously, IceTDrinker wrote…
struct that is versioned ?
this could be "TypeHashChanged" or "VersionedTypeHashChanged" since here it's not necessarily a struct, it can be an enum or an union.
a9c1977 to
6f8732f
Compare
SouchonTheo
left a comment
There was a problem hiding this comment.
@SouchonTheo made 36 comments and resolved 4 discussions.
Reviewable status: all files reviewed, 36 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and soonum).
utils/tfhe-backward-compat-checker/src/diff.rs line 20 at r8 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
yes maybe being explicit and calling this VersionsDispatchEnumChanged would be clearer
done
utils/tfhe-backward-compat-checker/src/diff.rs line 26 at r8 (raw file):
Previously, IceTDrinker wrote…
no EnumHashChanged ?
no VariantHashChanged ? (no idea if it's relevant)
could be good having comments explaining why we don't have some potentially
there is no hash of enum
utils/tfhe-backward-compat-checker/src/diff.rs line 32 at r8 (raw file):
Previously, IceTDrinker wrote…
variant : a variant from a VersionsDispatch enum ?
yep will change the name
utils/tfhe-backward-compat-checker/src/diff.rs line 37 at r8 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
this could be "TypeHashChanged" or "VersionedTypeHashChanged" since here it's not necessarily a struct, it can be an enum or an union.
ok
utils/tfhe-backward-compat-checker/src/diff.rs line 44 at r8 (raw file):
Previously, IceTDrinker wrote…
Upgrade code between two versions of a type ?
yes
utils/tfhe-backward-compat-checker/src/diff.rs line 141 at r8 (raw file):
Previously, IceTDrinker wrote…
neutral
maybe ?
is it always an addition if it's Ok ?
yep from what we discussed
I will change it
utils/tfhe-backward-compat-checker/src/diff.rs line 148 at r8 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
nice!
<3
utils/tfhe-backward-compat-checker/src/diff.rs line 150 at r8 (raw file):
Previously, IceTDrinker wrote…
a few comments to give an example of why it might be a warning and not a hard error, etc. could be nice I think
since this is the part that defines the rules on what is ok/warn/err
I push the explanation of each enum on the enum side
is this ok for you ? or do you want it at this lv ?
utils/tfhe-backward-compat-checker/src/diff.rs line 265 at r8 (raw file):
Previously, IceTDrinker wrote…
I don't think I asked to error on "unpredicted file", I think it's fine to ignore like you did before
ah ? I was sure you wanted to have a clean dir
I will remove the error
utils/tfhe-backward-compat-checker/src/diff.rs line 331 at r8 (raw file):
Previously, IceTDrinker wrote…
we do record a diff I think ?
since we seem to pass &mut entries to diff_enum
yes too much negation in comment lol
utils/tfhe-backward-compat-checker/src/report.rs line 10 at r8 (raw file):
Previously, IceTDrinker wrote…
errors, warnings, neutral
maybe ? not sure how it's called generally
let's go for neutral
utils/tfhe-backward-compat-checker/src/report.rs line 17 at r8 (raw file):
Previously, IceTDrinker wrote…
you don't check here if there were variants removals, today errors are only variants removal but I don't think we should specialize the logs if we don't do an analysis here
it's perfectly fine to say "errors detected, see: ... blablabla" to have the detail
ok so I did it like that if error where find
generic error message and ask to go to the error section
same for warning
utils/tfhe-backward-compat-checker/src/report.rs line 26 at r8 (raw file):
Previously, IceTDrinker wrote…
same
ok
utils/tfhe-backward-compat-checker/src/report.rs line 35 at r8 (raw file):
Previously, IceTDrinker wrote…
same "neutral changes detected, see full log:"
ok
utils/tfhe-backward-compat-checker/src/report.rs line 76 at r8 (raw file):
Previously, IceTDrinker wrote…
prefer using your mapping in code that exists for the various variants, here you end up with a different source of truth which will get out of sync
let's remove it
error warning and neatral is already define and display it as it is
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 184 at r6 (raw file):
Previously, IceTDrinker wrote…
maybe could emit a warning, since we ask stuff about versioning, if we don't find anything about versioning it's a bit suspicious
yep you are right
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 201 at r6 (raw file):
Previously, IceTDrinker wrote…
I think it would be good to error out if we have one and not the other
has_source != has_target
would be the corresponding weid case
will do a flag for this
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 325 at r6 (raw file):
Previously, IceTDrinker wrote…
I see another format below, it's just that I'm not clear as to what the non Adt case may be, and why you had a fallback here and a fallback below that is still present
forget to do it
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 119 at r8 (raw file):
Previously, IceTDrinker wrote…
why this change ?
probably not important, just curious, I don't know the internal rust compiler APIs
Yes super important
it's for the full path not only the name it was a mistake
.github/workflows/backward_compat_main_snapshot_consistency.yml line 45 at r8 (raw file):
Previously, IceTDrinker wrote…
is it the toolchain we need ?
there could be a make target that prints it for the first rust/rustup install, this way it is always in sync
cleaned
.github/workflows/backward_compat_main_snapshot_consistency.yml line 54 at r8 (raw file):
Previously, IceTDrinker wrote…
wondering if this script should have inputs
it should at least verify that the dirs exist
done
.github/workflows/backward_compat_pr_change_report.yml line 36 at r8 (raw file):
Previously, IceTDrinker wrote…
I think base is never used, so there is a problem in the logic of the workflow
I think the CI is doing the following :
do a report in the head dir between a head snapshot and the PR branch
while you might want (?)
do a report somewhere between the base branch snapshot and the PR branch snapshot
yes base is used
some env variable is used by default for the ci
this things will change I enforce the use of all the variable you will see it more clearly
.github/workflows/backward_compat_pr_change_report.yml line 45 at r8 (raw file):
Previously, IceTDrinker wrote…
maybe prefer cd in the script, I was lost
removed for cd
.github/workflows/backward_compat_pr_snapshot_consistency.yml line 34 at r8 (raw file):
Previously, IceTDrinker wrote…
this if should be removed
done
.github/workflows/backward_compat_pr_snapshot_consistency.yml line 36 at r8 (raw file):
Previously, IceTDrinker wrote…
why runs-on ? is it something that is heavy ?
replace by ubuntu you are right (copy/paste)
.github/workflows/backward_compat_pr_snapshot_consistency.yml line 38 at r8 (raw file):
Previously, IceTDrinker wrote…
concurrency is made for a push to main workflow, to be removed/reworked
removed
.github/workflows/backward_compat_pr_snapshot_consistency.yml line 45 at r8 (raw file):
Previously, IceTDrinker wrote…
why do we need a checkout token here but not in the other workflows ?
perhaps to be reworked
yes with all the change I might let an error I removed it no needed
.github/workflows/backward_compat_pr_snapshot_consistency.yml line 50 at r8 (raw file):
Previously, IceTDrinker wrote…
use a make command for this
done
.github/workflows/backward_compat_pr_snapshot_consistency.yml line 59 at r8 (raw file):
Previously, IceTDrinker wrote…
I think this workflow could be shared with the first one ?
since they seem to be doing similar stuff, but once on PR, once on merge to main
let's do it
.github/workflows/pr_snapshot_change_warning.yml line 45 at r4 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
What looks interesting to see here imo is the BASE_SNAPSHOT_DIR, not HEAD_SNAPSHOT_DIR. I think it makes sens to set this one here because BASE_SNAPSHOT_DIR is not inside the head folder, so if we change where it's cloned in this workflow it will not work anymore.
So I think it should be set as a parameter from this workflow.Since we run from the head folder, everything related to head is "self contained" so it make sense to have it in the makefile, but everything related to base is not so I think it should be defined at the workflow level.
I will make the argument mandatory to pass no doubt anymore !
utils/tfhe-lints/Makefile line 7 at r8 (raw file):
Previously, IceTDrinker wrote…
even though it calls dyling, this target is made to snapshot the crates, so maybe call the three targets
snapshot-$crate
I did something even smarter !
utils/tfhe-lints/Makefile line 12 at r8 (raw file):
Previously, IceTDrinker wrote…
the fact lints are used to generate snapshots is an implementation details, so I would call this run-all-snapshots
the comment can have information about the fact it uses dylint to run the snapshot
this will be updated following my changement
utils/tfhe-lints/Makefile line 47 at r8 (raw file):
Previously, IceTDrinker wrote…
Versioned types rather ?
ok
utils/tfhe-lints/Makefile line 55 at r8 (raw file):
Previously, IceTDrinker wrote…
is that HEAD_SNAPSHOT_DIR ?
same below
you are right better args
utils/tfhe-lints/Makefile line 61 at r8 (raw file):
Previously, IceTDrinker wrote…
this path looks to be incorrect ?
I don't see a base dir in your commit I think ?
Yep this is only for the CI
Maybe I will change it regarding the ci comment
utils/tfhe-lints/snapshots/lint_enum_snapshots_tfhe_csprng_base.json line 9 at r2 (raw file):
Previously, IceTDrinker wrote…
why did the hash change ? I don't recall if you updated the way structs are hashed
yep for the full path of the name so all struct changed
6f8732f to
bd29be3
Compare
soonum
left a comment
There was a problem hiding this comment.
Didn't look at the backward compatibility code.
Overall CI and build files look very neat 🐅
@soonum partially reviewed 30 files and made 7 comments.
Reviewable status: 30 of 35 files reviewed, 41 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and SouchonTheo).
.github/workflows/backward_compat_pr_snapshot_consistency.yml line 39 at r8 (raw file):
Previously, IceTDrinker wrote…
true for PR trigger, to check with @soonum
Not exactly, you have to check over ${{ github.event_name == 'pull_request' }}
.github/workflows/backward_compat_snapshot_consistency.yml line 33 at r9 (raw file):
runs-on: ubuntu-latest concurrency: group: ${{ github.workflow }}-${{ github.event_name == 'push' && github.sha || github.head_ref }}
Something feels not right about this group, maybe something like this would be more appropriate:
Suggestion:
group: ${{ github.workflow }}-${{ github.head_ref || github.sha }}.github/workflows/backward_compat_snapshot_consistency.yml line 34 at r9 (raw file):
concurrency: group: ${{ github.workflow }}-${{ github.event_name == 'push' && github.sha || github.head_ref }} cancel-in-progress: ${{ github.event_name != 'push' }}
Do we want to cancel the workflow on workflow_dispatch event ?
scripts/check_snapshot_consistency.sh line 14 at r8 (raw file):
echo "::error::Missing base snapshot: $base_file" mismatch=1 continue
Shouldn't we break here ? Unless a fast-path is not needed.
utils/tfhe-backward-compat-checker/Cargo.toml line 8 at r8 (raw file):
[dependencies] clap = { version = "4", features = ["derive"] }
Isn't version = "4" too broad for clap ?
I recall their interface could change drastically between minors. I might be wrong.
utils/tfhe-lints/common/Cargo.toml line 13 at r8 (raw file):
[dependencies] clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", rev = "54482290b5f32e6c6b57cc9e0a17153f432b0036" }
Does this link to a release commit ? If so, please add the version number with an inline comment.
SouchonTheo
left a comment
There was a problem hiding this comment.
@SouchonTheo made 6 comments.
Reviewable status: 30 of 35 files reviewed, 41 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and soonum).
.github/workflows/backward_compat_snapshot_consistency.yml line 33 at r9 (raw file):
Previously, soonum (David Testé) wrote…
Something feels not right about this group, maybe something like this would be more appropriate:
Done.
.github/workflows/backward_compat_snapshot_consistency.yml line 34 at r9 (raw file):
Previously, soonum (David Testé) wrote…
Do we want to cancel the workflow on
workflow_dispatchevent ?
I put it to play honstly the workflow_dispatch
scripts/check_snapshot_consistency.sh line 14 at r8 (raw file):
Previously, soonum (David Testé) wrote…
Shouldn't we
breakhere ? Unless a fast-path is not needed.
honestly I wanted to highlight everything which is not correct that's why we are not stoping
the aim was fix all of this and come back
utils/tfhe-backward-compat-checker/Cargo.toml line 8 at r8 (raw file):
Previously, soonum (David Testé) wrote…
Isn't
version = "4"too broad for clap ?
I recall their interface could change drastically between minors. I might be wrong.
idk honestly still a young man ahah
will do it to be safe
utils/tfhe-lints/common/Cargo.toml line 13 at r8 (raw file):
Previously, soonum (David Testé) wrote…
Does this link to a release commit ? If so, please add the version number with an inline comment.
I made a copy paste
.github/workflows/backward_compat_pr_snapshot_consistency.yml line 39 at r8 (raw file):
Previously, soonum (David Testé) wrote…
Not exactly, you have to check over
${{ github.event_name == 'pull_request' }}
I replaced it with cancel-in-progress: ${{ github.event_name != 'push' }}
in pr we cancel but not on main
bd29be3 to
eecf93f
Compare
SouchonTheo
left a comment
There was a problem hiding this comment.
@SouchonTheo made 1 comment.
Reviewable status: 30 of 35 files reviewed, 41 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and soonum).
utils/tfhe-lints/common/Cargo.toml line 13 at r8 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
I made a copy paste
ok it's the exact commit of clippy for dylint
I took the same as Nicolas
nsarlin-zama
left a comment
There was a problem hiding this comment.
@nsarlin-zama partially reviewed 8 files, made 3 comments, and resolved 2 discussions.
Reviewable status: 34 of 35 files reviewed, 40 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).
utils/tfhe-backward-compat-checker/Cargo.toml line 8 at r8 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
idk honestly still a young man ahah
will do it to be safe
actually, I think you should use the version from the workspace. Same for serde and everything that's already in the top-level Cargo.toml
utils/tfhe-lints/common/Cargo.toml line 13 at r8 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
ok it's the exact commit of clippy for dylint
I took the same as Nicolas
If I remember correctly this comes from an auto-generated file. The commit is updated automatically when we update the toolchain using the cargo dylint command, maybe it's worth checking this still works here
utils/tfhe-lints/Makefile line 47 at r11 (raw file):
.PHONY: backward-snapshot-base # Generate the base snapshot. Should be called when you modified enums, structs or upgrade related to Versioned types backward-snapshot-base: install_cargo_dylint install_tfhe_lints_toolchain
small nitpick, but I think it's better if all the targets use '_' and not '-' since it's the convention in the other makefile
IceTDrinker
left a comment
There was a problem hiding this comment.
Thanks we have a few last tiny things (and I made you do something that was not smart naming wise in hindsight, sorry)
@IceTDrinker partially reviewed 15 files and all commit messages, made 15 comments, and resolved 30 discussions.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on nsarlin-zama, soonum, and SouchonTheo).
.github/workflows/backward_compat_snapshot_consistency.yml line 33 at r9 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
Done.
I think we have this pattern in a few workflows, might have some stuff to update then @soonum
.github/workflows/backward_compat_snapshot_consistency.yml line 34 at r9 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
I put it to play honstly the
workflow_dispatch
Likely ok in this case, should not run for long (unlike benchmarks) @soonum
scripts/check_snapshot_consistency.sh line 14 at r8 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
honestly I wanted to highlight everything which is not correct that's why we are not stoping
the aim was fix all of this and come back
yep I like that approach 👍
utils/tfhe-backward-compat-checker/Cargo.toml line 8 at r8 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
actually, I think you should use the version from the workspace. Same for serde and everything that's already in the top-level Cargo.toml
yep prefer workspace deps
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 325 at r6 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
forget to do it
For my culture what other TyKind are there ?
.github/workflows/backward_compat_snapshot_consistency.yml line 23 at r11 (raw file):
- main paths: - 'utils/tfhe-lints/snapshots/lint_enum_snapshots_*.json'
I hope I did not suggest to check paths for the files, but I think we can always run the sanity checker for snapshots on main, it does not take much time and is a nice safety to have
scripts/check_snapshot_consistency.sh line 1 at r11 (raw file):
#!/usr/bin/env bash
I think you are re-implementing the diff program
example output
diff -q tfhe-rs tfhe-rs-other/
... truncated
Only in tfhe-rs: venv
Only in tfhe-rs: web-test-runner
user:~/Documents/zama/code$ echo $?
1
returns non 0 code on exit, there certainly is a summary mode
so I think you can get rid of this script and just use diff -q
utils/tfhe-backward-compat-checker/src/diff.rs line 34 at r11 (raw file):
}, /// A variant was removed from a versions dispatch enum. /// Existing serialized data referencing this variant can no longer be deserialized.
Removing a variant is a hard error ?
removing a variant is never legal ? (not sure)
utils/tfhe-backward-compat-checker/src/main.rs line 106 at r11 (raw file):
} if !args.allow_additional_enums && !neutral.is_empty() {
ok given the use here my neutral proposal from before does not feel right
are we certain we only have additions in the neutral category ?
I think having the neutral severity is OK (so no need to change things for severity)
but here the neutral name seems silly, so perhaps we stick to the additions naming (sorry) but the severity stays neutral ?
I just don't know if all neutral changes will always be additions ?
.github/workflows/backward_compat_pr_change_report.yml line 44 at r11 (raw file):
BASE_SNAPSHOT_DIR=../base/utils/tfhe-lints/snapshots \ HEAD_SNAPSHOT_DIR=utils/tfhe-lints/snapshots \ OUTPUT_FILE=../report.md
a tiny bit confused between the ../report.md
and the report.md below ?
utils/tfhe-lints/Makefile line 5 at r11 (raw file):
# Guard macro: fails with a clear message if a variable is not set # Usage: $(call require,VAR_NAME) require = $(if $($(1)),,$(error $(1) is required. Usage: make $@ $(1)=<value>))
curious about how this works ? I'm not a Make guru :)
utils/tfhe-lints/Makefile line 18 at r11 (raw file):
# Run all crates with a given dylint lib, $(1) = lib name, $(2) = optional data dir define run-all-crates
not sure if that's the best name
dylint_all ?
since we have a clippy_all in the main Makefile
does it run both the consistency checks and the snapshots ?
utils/tfhe-lints/Makefile line 19 at r11 (raw file):
# Run all crates with a given dylint lib, $(1) = lib name, $(2) = optional data dir define run-all-crates $(call run-dylint,$(1),$(2)) $(LINT_TFHE_ARGS)
I'm going to be annoying, I think it's interesting to have separate targets, when you dev on e.g. CSPRNG you only want to check CSPRNG e.g. and you don't have access to the target :)
you can keep the macros you defined, but having single targets can be useful for fine grained debug/lints
utils/tfhe-lints/Makefile line 54 at r11 (raw file):
backward-snapshot-head: install_cargo_dylint install_tfhe_lints_toolchain @echo "Generating head snapshot" @rm -rf $(SNAPSHOT_DIR)/head
should this require a HEAD_SNAPSHOT_DIR like you did below ?
nsarlin-zama
left a comment
There was a problem hiding this comment.
@nsarlin-zama made 1 comment.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).
utils/tfhe-backward-compat-checker/src/diff.rs line 34 at r11 (raw file):
Previously, IceTDrinker wrote…
Removing a variant is a hard error ?
removing a variant is never legal ? (not sure)
Only case where this might be legal is if done on a variant introduced and removed before a release.
In the other cases this is extremely dangerous because it means that we will have a version confusion. So technically this may be even worse than what the comment says since it might be possible to deserialize it, just not to the correct type.
|
Previously, IceTDrinker wrote…
in the end it seems this flag allow_additional_enums may not be useful |
SouchonTheo
left a comment
There was a problem hiding this comment.
@SouchonTheo made 14 comments and resolved 2 discussions.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and soonum).
.github/workflows/backward_compat_snapshot_consistency.yml line 23 at r11 (raw file):
Previously, IceTDrinker wrote…
I hope I did not suggest to check paths for the files, but I think we can always run the sanity checker for snapshots on main, it does not take much time and is a nice safety to have
no it was on purpose on my side
I will remove it if you want
Do you want to remove it in the other CI too ?
scripts/check_snapshot_consistency.sh line 14 at r8 (raw file):
Previously, IceTDrinker wrote…
yep I like that approach 👍
Done.
scripts/check_snapshot_consistency.sh line 1 at r11 (raw file):
Previously, IceTDrinker wrote…
I think you are re-implementing the diff program
example output
diff -q tfhe-rs tfhe-rs-other/ ... truncated Only in tfhe-rs: venv Only in tfhe-rs: web-test-runner user:~/Documents/zama/code$ echo $? 1returns non 0 code on exit, there certainly is a summary mode
so I think you can get rid of this script and just use diff -q
ack
utils/tfhe-backward-compat-checker/Cargo.toml line 8 at r8 (raw file):
Previously, IceTDrinker wrote…
yep prefer workspace deps
yes will do the change
utils/tfhe-backward-compat-checker/src/diff.rs line 34 at r11 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
Only case where this might be legal is if done on a variant introduced and removed before a release.
In the other cases this is extremely dangerous because it means that we will have a version confusion. So technically this may be even worse than what the comment says since it might be possible to deserialize it, just not to the correct type.
so I let it like that ? (that was my understanding)
utils/tfhe-backward-compat-checker/src/main.rs line 106 at r11 (raw file):
Previously, IceTDrinker wrote…
in the end it seems this flag allow_additional_enums may not be useful
removed the flag and keeped the test !
utils/tfhe-lints/common/Cargo.toml line 13 at r8 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
If I remember correctly this comes from an auto-generated file. The commit is updated automatically when we update the toolchain using the
cargo dylintcommand, maybe it's worth checking this still works here
if this work like that no need to worry in the end when we will update it will change accordingly
utils/tfhe-lints/snapshot/src/versions_dispatch_snapshot.rs line 325 at r6 (raw file):
Previously, IceTDrinker wrote…
For my culture what other TyKind are there ?
https://doc.rust-lang.org/nightly/nightly-rustc/rustc_type_ir/ty_kind/enum.TyKind.html
.github/workflows/backward_compat_pr_change_report.yml line 44 at r11 (raw file):
Previously, IceTDrinker wrote…
a tiny bit confused between the ../report.md
and the report.md below ?
I'm generating the report at the root of the ubuntu (because we are in head when we run the command)
utils/tfhe-lints/Makefile line 5 at r11 (raw file):
Previously, IceTDrinker wrote…
curious about how this works ? I'm not a Make guru :)
$($(1)) ->
if cond,then,else
the cond is evaluating this give somehing
no then bc everything is fine
else is an error to told the user to set the vars
utils/tfhe-lints/Makefile line 18 at r11 (raw file):
Previously, IceTDrinker wrote…
not sure if that's the best name
dylint_all ?
since we have a clippy_all in the main Makefile
does it run both the consistency checks and the snapshots ?
dylint_all is not the right name
in fine the run-all-carte is just the setup of the dylint for some rules
here we can run the snapshot or the lint rules or we also can do both
I would not rename it as it's a macro not callable from the outside
utils/tfhe-lints/Makefile line 19 at r11 (raw file):
Previously, IceTDrinker wrote…
I'm going to be annoying, I think it's interesting to have separate targets, when you dev on e.g. CSPRNG you only want to check CSPRNG e.g. and you don't have access to the target :)
you can keep the macros you defined, but having single targets can be useful for fine grained debug/lints
oki
utils/tfhe-lints/Makefile line 47 at r11 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
small nitpick, but I think it's better if all the targets use '_' and not '-' since it's the convention in the other makefile
you are right mb
utils/tfhe-lints/Makefile line 54 at r11 (raw file):
Previously, IceTDrinker wrote…
should this require a HEAD_SNAPSHOT_DIR like you did below ?
for those one no
we want to enforce it in the right place
e983096 to
b20b135
Compare
IceTDrinker
left a comment
There was a problem hiding this comment.
I think we are about to finish here :)
just one thing I would like you to check about the report (I have a doubt around the location it is saved in CI)
maybe you could error out if the file does not exist, because then it means something went wrong in the CI
so maybe you want a -f check instead of a -s
first : -f checks the file exist, then if it's not empty (-s check) you can display things
I think there was something about a comment where it does not convey enough that a variant removal is "a crime"
@IceTDrinker partially reviewed 8 files and all commit messages, made 4 comments, and resolved 7 discussions.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on nsarlin-zama, soonum, and SouchonTheo).
.github/workflows/backward_compat_snapshot_consistency.yml line 23 at r11 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
no it was on purpose on my side
I will remove it if you want
Do you want to remove it in the other CI too ?
we could, though in the case of the PR (for the other workflow) we know that the files changed or not, while merging on main we don't know what got merged before that could issues
utils/tfhe-backward-compat-checker/src/diff.rs line 34 at r11 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
so I let it like that ? (that was my understanding)
I think you may want to put a comment that indicates it's a much worse outcome than "no longer be deserialized" Nicolas is talking about version confusion, which is essentially type confusion (very dangerous for security)
.github/workflows/backward_compat_pr_change_report.yml line 44 at r11 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
I'm generating the report at the root of the ubuntu (because we are in head when we run the command)
since we did a cd we should be in the same dir ? I'm not sure, can you check the latest runs find the report ?
here it feels like (could be wrong) that report.md is one level above, but we are looking for the report in the current dir, so we are not updating the comment at the moment ?
nsarlin-zama
left a comment
There was a problem hiding this comment.
@nsarlin-zama partially reviewed 4 files, made 2 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).
utils/tfhe-backward-compat-checker/src/diff.rs line 34 at r11 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
so I let it like that ? (that was my understanding)
That's what I would do, yes
utils/tfhe-lints/common/Cargo.toml line 13 at r8 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
if this work like that no need to worry in the end when we will update it will change accordingly
I just tested, it looks like it works, but you need to put clippy_utils and dylint_linting in the Cargo.toml above in a [workspace.dependencies] section, and then use { workspace = true } here (and in the other lints)
b20b135 to
46ec630
Compare
… and upgrade information hashed
46ec630 to
1ee149d
Compare
SouchonTheo
left a comment
There was a problem hiding this comment.
yep ! I will do something to be super safe
@SouchonTheo made 5 comments.
Reviewable status: 28 of 35 files reviewed, 8 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, and soonum).
.github/workflows/backward_compat_snapshot_consistency.yml line 23 at r11 (raw file):
Previously, IceTDrinker wrote…
we could, though in the case of the PR (for the other workflow) we know that the files changed or not, while merging on main we don't know what got merged before that could issues
yep
utils/tfhe-backward-compat-checker/src/diff.rs line 34 at r11 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
That's what I would do, yes
ok will add something like you suggest
utils/tfhe-lints/common/Cargo.toml line 13 at r8 (raw file):
Previously, nsarlin-zama (Nicolas Sarlin) wrote…
I just tested, it looks like it works, but you need to put clippy_utils and dylint_linting in the Cargo.toml above in a
[workspace.dependencies]section, and then use{ workspace = true }here (and in the other lints)
done
.github/workflows/backward_compat_pr_change_report.yml line 44 at r11 (raw file):
Previously, IceTDrinker wrote…
since we did a cd we should be in the same dir ? I'm not sure, can you check the latest runs find the report ?
here it feels like (could be wrong) that report.md is one level above, but we are looking for the report in the current dir, so we are not updating the comment at the moment ?
ohhhh
yes you are right I was thinking of the other step not the other one
when I modified with -s and -f I made the fix my bad
IceTDrinker
left a comment
There was a problem hiding this comment.
@IceTDrinker partially reviewed 7 files and all commit messages, made 4 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on soonum and SouchonTheo).
.github/workflows/backward_compat_snapshot_consistency.yml line 23 at r11 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
yep
one thing to keep in mind is if we change the location of the snapshots then the trigger in the other workflow won't work, but it's not a concern currently
.github/workflows/backward_compat_pr_change_report.yml line 44 at r11 (raw file):
Previously, SouchonTheo (Theo Souchon) wrote…
ohhhh
yes you are right I was thinking of the other step not the other one
when I modified with -s and -f I made the fix my bad
I think the current version is wrong, it says there is no report if -f ../report is true
my thinking reading your current code is something like this :
I think I would write the code as the following :
if [ -f ../report.md ]; then
if [ -s ../report.md ]; then
# size is non zero, we have a report
has_report = true
else
has_report = false
fi
else
echo "::error::report.md was not created — something went wrong"
exit 1
fithe above is pseudo code so not fully correct
one thing is likely to set a variable to ../report.md to avoid repeating the same name every time
we could have a protection rule on that step so that we are sure the step ran properly if needed (I can set this up later)
utils/tfhe-lints/common/Cargo.toml line 14 at r16 (raw file):
[dependencies] clippy_utils = { workspace = true } dylint_linting = "5.0.0"
can use workspace I think ?
.github/workflows/backward_compat_pr_change_report.yml line 20 at r16 (raw file):
jobs: change-report: name: backward_compat_pr_change_report/change-report
since we are going to verify that this step ran successfully later (verify the report is correctly generted), can we have (bpr) added in the name, it's a way we have to find branch proections rules more easily
example we have in the lib:
jobs:
backward-compat-tests:
name: aws_tfhe_backward_compat_tests/backward-compat-tests (bpr)
PR regarding lints
CI
New CI file to run the checker in the PR
It will return an error if
tfhe-lints
Now tfhe-lints contain 3 crate
(AI was used to wrote some documentation)
Checker
there is also a new crate called checker where the check happen
you have all the rules we need to follow and everything is tested
(AI was used to generate test case)
Makefile
The makefile was updated to only run the right lib
Also a new Makefile arrived
Makefile.compatinside it you have everything you need to generate or check the stuff you need (AI was used to help me create some rules)
This change is